map will generate a new array reference every time the store changes which may cause unnecessary rerenders. Maybe we can use elector getBlocksByUID which is cached and just that array down.

dcf20eb

difference( selectedBlockNames, allowedBlockNames ).length === 0; seems like a simple way to check if all all items of selectedBlockNames are contained in allowedBlockNames.
But it makes people reading the code having to think a little bit about what the code is doing. Maybe we can have a simple function arrayContainsArray or something similar and use it in shouldRenderItem plus a comment saying that item is rendered if allowedBlocks is not set or if all selectedBlocks are included in allowedBlocks.

bada6cc

Plugins may want to have multiple menu items and even have their own separators to separate their items. In that case, they would need to be repeating the allowedBlocks prop on each item plus use the BlockSettingsMenuPluginsGroup to pass the separator.
Maybe we can consider exposing the slot in a way that does not render anything and receives the allowed block props. And exposes a simple component called SettingMenuItem that just uses IconButton and handles the mobile label logic.
E.g:

<PluginBlockSettingsMenu allowedBlocks={ [...] }>
<BlockSettingMenuItem
	icon={}
	label="Item of the first group"
	onClick={}
/>
...
	<div className="editor-block-settings-menu__separator" />
<BlockSettingMenuItem
	icon={}
	label="Item of the second group"
	onClick={}
/>
...
</PluginBlockSettingsMenu>

This would also allow plugins more options to customize their UI.

I like the idea of enabling plugins to introduce more than one item on the block settings menu.

How about this API?

<BlockSettingsMenuPluginsItem 
            allowedBlockNames=[ 'core/paragraph' ]
            secondaryMenu=[
                {
                  label: 'Secondary 1',
                  onClick: { secondary1OnClick }
                },
                {
                  label: 'Secondary 2',
                  onClick: { secondary2OnClick }
                },
            ],
            icon='dashicon-name'
            label='Plugin name'
            onClick={ primaryOnClick } />

The idea would be to enable plugins to add either one or multiple items, in an browsers-like fashion (compare LastPass VS Save to Pocket items):

If BlockSettingsMenuPluginsItem receives a secondaryMenu property it'll introduce a secondary menu and an arrow in the item to communicate there is another level. If secondaryMenu is not present it'll render a normal item. On mobile, we may want to use a different pattern (for ex: render the two items consecutively in the primary menu).

This doesn't address allowing plugins to add separators. I lean towards thinking separators should be UI only core can add - by staying the same no matter the plugins an user has installed Gutenberg communicates clearly how the menu is organized.

What do you think?

Hi @nosolosw, I like the idea of submenus so plugins can group their actions if they have multiple operations.
This proposal introduces a new type of menus that we don't have in Gutenberg yet cc: @karmatosed .

Regarding the technical point of view:
What if we have a component that allows the rendering of multiple items like:

<MultipleItemsMenu
	icon='dashicon-name'
    label='Plugin name'"
>
	<MenuItem
		label="Secondary 1"
		onClick={}
	/>
	<MenuItem
		label="Secondary 2"
		onClick={}
	/>
</MultipleItemsMenu>

It would be a simple UI component that even core could use.

If we allow plugins to render what they want to the slot, then they could use this component as:

<PluginBlockSettingsMenu allowedBlocks={ [...] }>
<MultipleItemsMenu
	icon='dashicon-name'
    label='Plugin name'"
>
...
</PluginBlockSettingsMenu>

Both approaches are similar both if we use react components we have some things simplified.
Using secondaryMenu=[ { label: 'Secondary 1', onClick: { secondary1OnClicl } ..., Will create a new array reference and new object references per item inside the array. For plugins to avoid rerenders they will need to have some logic to cache the references and only update the references when onClick or labels change. If we represent the same info as React components that logic is not needed.

As per private conversations with Tammie, she suggested that plugins shouldn't be able to have the flexibility to customize the slot at this point (adding separators, etc).

There are a couple of extra considerations we haven't considered yet and we should take into account:

• the API surface of this should be similar to the one of PluginSidebarMenuItem, as to create a coherent experience for developers.
• plugins may want to have a different set of allowedBlocks per item.

All things considered, I think we should go with the API as it is in the PR, as it meets all requirements (plugins can have more than one item, items can have different allowedBlocks sets, developers can't add their own separators/components, and the API shape is the same than the PlugiSidebarMenuItem). I still think a component for secondary menus could be useful, but that can be added later if it proves necessary/useful.

Thanks for your time, Jorge! This conversation has helped me to understand better the Gutenberg APIs.

Normally we start the extensibility slot names with Plugin e.g: PluginPostPublishPanel, PluginPostStatusInfo etc... So it may make sense to call this component something like PluginBlockSettingsMenu.

1237f0b

As per #7895 (comment) reverted this change: the external API lives now in edit-post/block-settings-menu and can be used as PluginBlockSettingsMenuItem.

compose will call: onClick( onClose( ...arg ) ); I think what we want here is:

( ...args ) => {
	onClick( ...args );
	onClose();
}

Note that the onClick API isn't passed any props, so

onClick( onClose() );

VS

( ) => {
	onClick( );
	onClose();
}

is essentially the same (omiting the sequence they're called, which doesn't matter as closing and executing the action are parallel processes).

I thought we passed arguments to onClick, if we don't pass them it is exactly the same.

All plugin specific code should live outside of editor folder. This is purely design decision. The idea is that editor is meant to provide all the pieces to required to build your own editor interface. edit-post is one of the examples of this implementation. You can check how this was done for publish panels in here:
https://github.com/WordPress/gutenberg/blob/master/editor/components/post-publish-panel/index.js#L88-L99

Related PR and discussion: #6798.

See also related discussion related to using slots in here:
#7199 (comment)

There are a few important point discussed and you can also learn why:

<_BlockSettingsMenuFirstItem.Slot fillProps={ { onClose } } />

exists in the first place and why it is considered as internal implementation.

01b045b takes this and #7895 (comment) into account and exposes the API as PluginBlockSettingsMenuItem.

PluginPrePublishPanel which I referenced as an example is fully defined inside edit-post module, and passed down to the component imported from editor module as a prop. Is there any reason we shouldn't be doing the same in here?

I think I've addressed this concern with the latest changes, editor and edit-post are decoupled now.

Wouldn't it be more flexible if produce the following code:

<div className="class-name...">
    { fills }
</div>

In More Menu we don't use explicit DOM elements as separators, we rather use CSS styles to achieve a similar result. In this scenario, the placement of separator is fully dependent on the sibiling componets which is not optimal.

Note that this is how the BlockSettingsMenu works at the moment, which is different from the MoreMenu. I'd like to leave this out of this PR as it's a different concern.

getBlocksByUID was deprecated in Gutenberg v3.3 and is slated to be removed in the next release. A warning would have been logged to your console on its use.

Similarly, wp.element.compose was deprecated in v3.3 and is slated to be removed in the next release. See #7948